-
Notifications
You must be signed in to change notification settings - Fork 125
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
PR Preview - StorybookThis pull request preview deployment is now available. ✅ Preview: exp+io.showtime.storybook://expo-development-client/?url=https://u.expo.dev/e77d5b68-bb27-45da-aa5c-96c1fdbf6706&channel-name=pr-1053 Comment ID: 1110887695 |
PR Preview - Storybook on Chromatic |
PR Preview - AppThis pull request preview deployment is now available. ✅ Preview: exp+io.showtime.development://expo-development-client/?url=https://u.expo.dev/45cbf5d5-24fe-4aa6-9580-acf540651abd&channel-name=pr-1053 Comment ID: 1110934196 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Wow! looks clean. 💯
LGTM! 👍 |
addSuffix: true, | ||
} | ||
)}`} | ||
{nft?.token_created && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I know using &&
operator in renders might cause unexpected results. Not sure if it can be 0 (zero) but even if not, it would be good practice to use Ternary Operator for the values comes from the endpoint which we are not sure it will be 0
or undefined
.
Ref: https://kentcdodds.com/blog/use-ternaries-rather-than-and-and-in-jsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, thanks! Is there an ESLint plugin for this? I keep forgetting about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there is but it's not specific to JSX, we should still be able to use the &&
operator. I'll check it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Included this change in my latest PR
@@ -31,7 +31,7 @@ export function ModalSheet({ visible = true, bodyContentTW, ...props }: Props) { | |||
<Modal | |||
key={`modalsheet-${props.title}-lg`} | |||
title={props.title} | |||
close={() => { | |||
onClose={() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌🏽
Screen components are looking very light weight now with the refactored |
Why
We have a new Modal Screen (thanks to @gorhom) 🎉 This PR implements the new HOC on all modals. The modal screens are now very clean and all share the same logic
How
Modal
,ModalSheet
andModalScreen
) implementationLogin
,Followers / Following
,Create
)Comments
modal "No comments yet…" stateTest Plan
Run the app and check all the modals, on all platforms and all screen sizes